Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

thanks a lot for your contribution @blfrantz !

As you mention this is something aggregation specific. Adding it to handler.js means it will only work with pie, area, line, bar, heatmap and gauge charts. It won't with all the others (maps, metric, tagcloud, more to come, any 3rd party visualizations).

I would suggest implementing this as an aggregation option. The date histogram aggregation config and logic is in ui/agg_types/buckets/date_histogram.js. This way this option can be completely independent from the visualization.

The actual conversion of data happens in tabify response handler, this is probably where you should check for this option and skip partial buckets. ui/vis/response_handlers/tabify.js and basic.js in same folder use the tabify functions defined in ui/agg_response/tabify.

let me know what you think and if this is helpful.

Thanks @ppisljar! Sounds like good advice. I like the idea of putting this with the date_histogram aggregation instead of the axis config. I was thinking this was more of an axis display option which led me to my implementation, but yours should be a lot cleaner. I'll work on this and update the PR shortly (doing this in my free time so it may be a few days).

Hey @ppisljar, now that I've gotten into it I have a question: is it possible to get the time range covered by the query from within the response_handlers? It seems like this information should be knowable since the exact timestamps are specified in the "range" portion of the _msearch query, but I can't find where to get this from the vis object. My issue is that without knowing the range of time included in the query, I can't know for sure which buckets are indeed "partial." I don't want to just assume the first/last ones are and drop them every time since absolute time ranges could be expected to align with the buckets and not have any partials.

The nice thing about my first approach, ugly as it was, is it had access to this range info and could use the same logic as the "this area contains partial data" tooltip (which incidentally also made it easy to extend that tooltip to say something more relevant in this case).

Any suggestions? I do like the genericism of your suggestion, but it's not clear to me where to get this range information.

Edit: One other nice thing about editing the data in handler.js and treating this as a view option rather than an aggregation option, is we don't need to requery when toggling this setting. But that's minor.

@ppisljar, any thoughts? Thanks!

sorry for late reply.

in your response handler you can access the timefilter on vis.filters.timeRange

preferably you would be able to figure this out of elastic search response, but i am not sure if that is possible.

No problem, @ppisljar .

So in the relative search case, timeRange only contains something like {from: "now-15m", to: "now", mode: "quick"}. Without knowing when the query fired, that's not really useful (and even if I did, it'd be ugly to calculate). As for the ES response, the data returned by the _msearch query doesn't appear to have any timestamp info aside from the bucket times, but those don't necessarily align with the queried range (if they did, this whole feature would be unnecessary).

Unless this information is available somewhere I'm not aware of (or not that hard to pass in), I'm beginning to think my original approach may be more realistic. In the same way that the "partial data" tooltip only shows up for some types of visuals (not all where it could be relevant), I'm wondering if having this feature exist only for some of the relevant cases (arguably the ones that matter most) is enough for now? What do you think? If you're ok with that approach, can you provide feedback on my 4th question (about test frameworks)?

Thanks!

import { getTime } from 'ui/timefilter/get_time';

const parsedTime = getTime(vis.indexPattern, vis.filters.timeRange);

let me know if this helps

@ppisljar thanks for that tip, it worked great. I've updated the PR with the new implementation, let me know what you think.

Thanks @ppisljar. That makes sense, and does make the code organization more elegant/intuitive. However I'm finding that it makes the dropPartials function itself rather more complicated and therefore more brittle.

Because of the way the tabified data is structured, things that were simple become a bit complicated in the proposed approach.

In the no-split-charts case, the following works (called with dropPartialRows(write.response(), write.timeRange) toward the end of tabifyAggResponse() in tabify.js:

function dropPartialRows(tableGroup, timeRange) {
  tableGroup.tables.forEach(table => {
    if (table.rows.length < 2) return;

    const aggIndex = _.findIndex(table.columns, {
      'aggConfig': {
        '_opts': {
          'type': 'date_histogram',
          'params': {
            'field': timeRange.name,
            'drop_partials': true
          }
        }
      }
    });

    if (aggIndex !== -1) {
      const time0 = table.rows[0][aggIndex].key;
      const time1 = _.find(table.rows, r => {
        if (r[aggIndex].key !== time0) return true;
        else return false;
      })[aggIndex].key;

      const interval = time1 - time0;

      table.rows = table.rows.filter(arr => {
        if (arr[aggIndex].key < timeRange.gte) return false;
        if (arr[aggIndex].key + interval > timeRange.lte) return false;
        return true;
      });
    }
  });
}

That's not too bad, but some things that could be simple (like determining the interval) get a bit complicated because tabify's rows array contains a separate row for every value on each series, which means there can be multiple rows per time bucket, which means I have to scan the array until I find a new time to determine the interval. Previously, I could just compare the first and second items in the array.

Furthermore, the above code doesn't handle the split charts case (whereas the current PR code does), because in that event tableGroup contains tables of tables, and so I'd need to process this recursively or something. That's not a big deal, but as this gets more complex (and computationally expensive) I wanted to see if you thought this was still the best approach. It also seems harder to test. Apologies if I'm missing something. Thanks!

@ppisljar Thanks, I've cleaned up the conditionals for _dropPartials, let me know how it looks now. I still need to rebase/retest before this is ready to pull but waiting to see if any further tweaks are needed per @timroes comments.

@ppisljar & @timroes: I just force-pushed a cleanup of the commit history to remove all the old iterations. As part of this I added the new tooltip text recommended by @gchaps. I also just rebased onto the latest upstream, fixed a couple of my tests that were broken by some recent upstream changes, and sanity tested that the feature still works as expected. Awaiting any further instructions. Thanks!

@ppisljar & @timroes: I know you guys are busy, but just wanted to make sure this was still on your radar. Thanks!

Jenkins, test this

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

@markov00 Thanks for the feedback and tip about that test failure. I've merged and made the requested fixes. I also confirmed that the failing Shakespeare test passes locally now.

jenkins test this

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

jenkins test this

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request

jenkins test this, want to be sure it was a flaky CI test failure

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request

What's the next step here?

Jenkins, test this - I'll give it a last test run, then will merge it.

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request

Hi Brian,

thanks a lot for your first contribution to Kibana! I just merged your commit on master and it will be backported to 6.x, so that these changes will be released in 6.5.

Cheers
Tim

Awesome, thanks everyone!

@timroes or @markov00 or @ppisljar can any of you help me understand why I don't see this option here on this 6.5.0 visualization? This one is using ecommerce sample data, and I also tried a new line chart from makelogs data.

@LeeDr #25520

Pinging @elastic/kibana-app

We just updated to 6.5.2, and I still don't see the checkbox option, even for a newly created visualization.
According to the release notes, #25520 should be included in 6.5.2?

@Benny-Git unfortunately that PR was missing the backport to 6.5 😞 This should be released in a future 6.5 patch release instead, sorry.

@gchaps Could you perhaps remove Fixes option for showing partial buckets #25520 from the 6.5.2 release notes?

@timroes thanks for the very quick response.
I'll just manually update my few relevant saved objects with "drop_partials": true for now :)

@timroes Done.